I'm also considering that the breaking change you are making (switching a property from required to optional) is not allowed if the structure you're doing this in is exposed to be read by customer code somewhere.

Unfortunately, I don't think there is any other way to implement this feature within the current code as containerPort and containerPortRange are mutually exclusive. I tried more complicated solutions involving two interfaces and type unions but then jsii is incompatible, so I'm stuck 🤷‍♂️

I agree this is not optimal. Something we could consider is passing values through the token system. We could do something like:

class PortRange implements cdk.IResolvable {
  public readonly creationStack: string[] = [];
  public readonly typeHint?: cdk.ResolutionTypeHint;
  public readonly isPortRange = true;

  public static of(startPort: number, endPort: number): number {
    return cdk.Token.asNumber(new PortRange(startPort, endPort));
  }

  public static tryDetect(port: number): PortRange | undefined {
    const x = cdk.Tokenization.reverseNumber(port);
    return x && (x as any).isPortRange ? x as PortRange : undefined;
  }

  private constructor(public readonly startPort: number, public readonly endPort: number) {  }

  public resolve(_context: cdk.IResolveContext) {
    // This should not end up in a CFN template, so does not need to implement
    // resolve()
    throw new Error('PortRange may only be passed to ECS PortMapping\'s containerPort');
  }

  public toString(): string {
    return cdk.Token.asString(this);
  }
}

This is using some dark token magic, but it will work. We can use that to smuggle a PortRange object in the containerPort variable that normally accepts a number.

We would use that instead of adding a new property.

Demo:

const x = PortRange.of(1000, 2000);
console.log(x);
// -1.8881545897087535e+289

console.log(PortRange.tryDetect(x));
/*
PortRange {
  startPort: 1000,
  endPort: 2000,
  creationStack: [],
  typeHint: 'string'
}
*/

Or the following, which is not as nice but much more simple:

const portMapping: PortMapping = {
  containerPort: 0,  // Magic value
  portRange: '1000-2000',
};

Potentially with a const to give a nice name to the 0.

This is probably preferable for understandability and discoverability.

Or the following, which is not as nice but much more simple:

If I understand correctly, you would leave containerPort non-nullable and you would ask users to set it to a magic value when they want to use the containerPortRange, right? Doesn't this complicate things for the end user as he needs to know about this quirk?

f I understand correctly, you would leave containerPort non-nullable and you would ask users to set it to a magic value when they want to use the containerPortRange, right?

Indeed.

Doesn't this complicate things for the end user as he needs to know about this quirk?

Yes, but it's not all that bad.

• First of all, we will put an example in the README, and 90% of users will just copy/paste that.
• Second, we will put this information in the docstring of both properties, so anyone inspecting the API or reading the popup in their IDE will see what needs to happen.
• Third, we will put in runtime validation saying if you supply 'portRange' you must set containerPort to 0 and if you set containerPort to 0, you must supply portRange, making it very clear what the deal is.

As much as the trick feels neat, this is more discoverable than pulling in the magic PortRange class.

I have to admit that, before opening this PR, I thought a lot about adding a PortRange class with named constructors, but I didn't know it was possible to do it using tokens as backing store. As a user, I find suboptimal to have to know that there is a magic value that changes the behaviour of the object, but on the other side, having to know that a property typehinted as number magically accepts a value obtained from a cdk.IResolvable object is suboptimal too. I pushed the changes and the PR is ready for another round of review. In a future major version of CDK it may be cool to revert to the solution of two nullable properties, if BC is allowed in that case.

I have implemented all the requested changes. There are still some outstanding points above, feel free to mark the conversations as done if you are satisfied with the replies or add more feedback on how I should proceed.

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

It seems that Mergify can't rebase the PR because, while it was open, the GitHub workflows were changed, and the app does not have the appropriate permissions to update them. Should I rebase manually?

Build is now failing because of different NodeJS version used for a resource in the integration test I've added. Do I have to update the snapshot or can you do it for me?

Thanks for updating the test snapshot. Unfortunately, there is one more problem that makes the build fail, but seems to be unrelated 😭 Maybe another pull of the changes from the main branch can solve it?

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: aa7cb6d
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).